Isn't this something that's checked by the token?

this was updated to

        // This call verifies that `from` is the owner of `tokenId` (in `_onSend`), and the previous checks ensure
        // that `spender` is allowed to move tokenId on behalf of `from`.
        //
        // Perform the crosschain transfer and return the handler

The first part is indeed checked by the token. That is what I'm saying (maybe it could be more explicit).
The second part is not checked by the token. The token check that the spender he sees is allowed. That is the bridge itself. So the token will check that the owner authorised the bridge, and the bridge will do an additional check that whoever did the crosschainTransferFrom is also authorised by the owner of token. That last check is not (and cannot) be done by the token itself.

I see so this allows authorized ERC-721 spenders to also do cross-chain transfers.

This needs to be documented in the interface of crosschainTransferFrom.

Why is this line required? I see no relation with its documentation

If we don't have it, slither will be unhappy. This is because the IERC7786Recipient.receiveMessage is payable (to accomodate gateways that want to send native token.

In our case, we could teoretically get ether in, and there would be no way of getting it out. Slither is unhappy about that, and this is how we silent it.

Interesting. I'm curious why slither is not unhappy about the ERC7786Recipient contract since it also implements the receiveMessage payable. Maybe it's because BridgeERC721 implements processMessage without handling value, so there are no other functions left to handle the native value received in receiveMessage.

I agree BridgeERC721 shouldn't handle value at all, but, maybe we should restrict value sent in receiveMessage? E.g.:

function _processMessage(
    address gateway,
    bytes32 receiveId,
    bytes calldata sender,
    bytes calldata payload
) internal virtual override {
    require(msg.value < 1);
    super._processMessage(gateway, receiveId, sender, payload);
}

I think its because ERC7786Recipient (and BridgeERC721) have unimplemented functions. Slither doesn't know what they do, and if they are going to move ether out, so I guess its not flagging them.

maybe we should restrict value sent in receiveMessage

If we do that, and if the gateway ever decides to send some value, then we would not be processing the message, and the ERC-721 NFT would not be minted/released. Just becaused we receive either we don't know how to handle, we would be posibly bricking a token. Sounds like a disaster waiting to happen.

Also, what if someone wants to override the bridge to add more logic that deals with ethers. Some admin may add a drain, and that is fine. Other may want to add more code that deals with the token as a payment, and automatically move it somewhere or something.

I wouldn't add a check here.

Yeah, got it. So the receiving side may require value to process the message (e.g. charging the gas for passing the message).

Let's not add the payable check, but I still think Slither has a good point, and we should document that if the underlying bridge sends value to receiveMessage, such value will indeed be stuck.

Shouldn't this contract implement onERC721Received? How's the bridge able to receive the tokens?

This is an unsafe transfer, to it will work without the onERC721Received hook present in the bridge.

We used to have a onERC721Received hook that allowed anyone to do a safeTransferFrom to the the bridge, and pass in data that contained the to (in ERC-7830 format). That way you were able to do a crosschain send in just one operation, without the approval+transfer pattern.

We decided to remove that because of security issues. Not on the contract side, but on the wallet side. Basically, if an app/UI asks your wallet to do a safeTransferFrom to the bridge, you'll see you are sending the token to the bridge (which is good), but its likelly you won't see the data part of the transfer. If the app/UI puts the wrong data (either maliciously or by mistake) you are going to send you token to some unexpected recipient, and the wallet won't show that information to you.

@frangio

Basically, if an app/UI asks your wallet to do a safeTransferFrom to the bridge, you'll see you are sending the token to the bridge (which is good), but its likelly you won't see the data part of the transfer.

I see. So one thing is actually using the data argument to allow one-step crosschain transfers, but another is guaranteeing that the recipient can indeed receive the token. One thing that may happen is that the message is processed and the token is effectively sent to an address that can't transfer it out. Wouldn't be better that the bridge keeps custody in those cases?

Its not really about "guaranteeing that the recipient can receive the token". Its about guaranteeing that the sender sees (and can verify) who he is sending the token to. I believe the sender should not be "blindly" signing the crosschain recipient. If the wallet doesn't display the "data" section that his happening.

In particular, there is no way for a bridge to distinguish between:

• the EOA of the (valid) receiver
• the EOA of an attacker that injected malicious "data", that the user was not able to review because the wallet hides it
• an invalid address that has no code, and that doesn't have a (known) private key.

One thing that may happen is that the message is processed and the token is effectively sent to an address that can't transfer it out. Wouldn't be better that the bridge keeps custody in those cases?

We briefly discussed this before and landed on transferFrom because it seems to require fewer assumptions for recovery. The bridge keeping custody is only better if the message can be retried after the receiver has been upgraded with the receive handler. If you do transferFrom, you just need the receiver to be upgradeable.

An alternative path for recovery would be if the failure to deliver the message could be reported back in the source chain. But these are strong assumptions that 7786 doesn't guarantee at all.

Edit: Just saw this was discussed in another thread #6259 (comment)

It would be easy to decode this in calldata afaik

require(payload.length > 9f); // fromOffset + toBinaryOffset + tokenId + fromLength + toBinaryLength
uint256 fromOffset = payload[:0x20];
uint256 toBinaryOffset = payload[0x20:0x40];
require(fromOffset > 0x5f && fromOffset <= payload.length && toBinaryOffset > 0x7f && toBinaryOffset <= payload.length);
uint256 tokenId = payload[0x40:0x60];
uint256 fromLength = payload[fromOffset:fromOffset + 0x20];
uint256 toBinaryLength = payload[toBinaryOffset:toBinaryOffset + 0x20];
require(fromOffset + fromLength <= payload.length && toBinaryOffset + toBinaryLength <= payload.length)

So we don't allocate extra memory

Readability is not great. I think in the context of a "we are moving tokens" operation, the extra gas cost here is not that bad.

If we really want to micro optimize this, I would first merge the "simple solidity" version, and then do small PR that incrementally optimise. That way we can leverage our tools for gas comparison.

Shouldn't we use _safeMint?

I think we used to, and we had a long discussion about that with @frangio.

Basically, if the recipient is not "equiped" to receive tokens, what do we want to happen ?

Option 1

We do a _safeMint (on ERC721Crosschain) / a safeTransferFrom (on BridgeERC721). This causes the ERC7786.receiveMessage to fail. This likelly goes back to the gateway. This token is not moved. To fix the situation, you need to upgrade the receiver to add the required logic, and the re-do the call at the gateway level.

• if you cannot upgrade the receiver, the token is lost forever
• If for some reason the upgrade took to long, and some expiry thing on the gateway prevents you from replaying, the token is lost forever.

Options 2

we do a _mint (on ERC721Crosschain) / a transferFrom (on BridgeERC721). This forces the token transfer. From a gateway point of view, everything is fine. If the receiver doesn't know how to handle the token it just received, you need to do an upgrade to had that functionnality

• if you cannot upgrade the receiver, the token is lost forever

In both cases, an upgrade is needed. But option 1 also adds more logic to replay the message after the upgrade. @frangio and myself thought option 2 is probably better.

I was thinking a 3rd option is to include a "reverse" function (or similar) that cancels out the message delivery if it's not executed in X time. While I agree it's an opinionated decision and may not be the best default, I feel the BridgeERC721 is incomplete if we either suggest a workaround or implement one by default.

To be clear, I overall agree that option 2 is simpler give the case of a stuck token, but may be worth considering a default functionality that lets developers handle it properly. For example, see that ERC721Wrapper has a _recover internal function.

Option 3 comes with its own set of problem.

It could be implemented as an extension, but I don't think this complexity should be in the default.

Lets discuss that on our weekly call.

I'm open to an internal _recover function that maybe gets unlocked for a token id once a transfer fails.

My preference would be for the simpler approach that just does transferFrom. But this is not an option in ERC-1155 (#6281).